-
-
Notifications
You must be signed in to change notification settings - Fork 278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Pass omit_default_port from request to response #809
Pass omit_default_port from request to response #809
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I think that all makes sense. Just to make sure I'm getting it though, is it accurate to say that the logs you show here are the pre-patch behavior? ie I would think if I understand the change correctly that after the patch the request/response values should actually match up (with either both or neither containing the port, depending on the omit_default_port setting). Is that accurate?
Yes this is correct. |
6b009a5
to
e7e30a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
@mburumaxwell did you need a release with this change? |
@geemus it would be great to get a release. A patch maybe? |
Released in v0.97.2. @mburumaxwell Random thought, I wonder if we might eventually want to move toward something like a |
That would actually make things a lot easier because the information required for logging/instrumentation or other work would always be available. It makes a lot of sense. |
@mburumaxwell - thanks for confirming. It seemed like it could in the long term make things much simpler and would allow any other changes to request to "just work". The in between parts seem like they could be a bit trickier though. I'll have to think a bit more on the specifics, but it's good to have a general idea of direction at least. |
I have a random, possible not so-good, idea:
Problem is it would break the functionality but maybe the reverse would work i.e. opt-in the new behaviour. Also, thinking out-loud. |
@mburumaxwell - I created a new issue with more details of this idea #811 so that we can discuss (and hopefully not forget) as needed. No action for now, just wanted to keep you in the loop. |
Similar to #803, this PR passes the
omit_default_port
attribute to the response.Why?
When subscribed via ActiveNotifications, the Url formed for the request is not similar to the one for the response. The later includes the port. E.g. https://contoso.com:443/my-path
The URLs are generated using
Excon::Utils.request_uri(...)
The code:
The logs
Related to dependabot/dependabot-core#6459
I also fixed indentation for additions in #803